-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[Security] Add tokenSource
parameter for CSRF token validation sources
#21363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f7ce0e8
to
d885b0c
Compare
d885b0c
to
f2a80ed
Compare
|
||
* ``IsCsrfTokenValid::SOURCE_PAYLOAD`` (default): request payload (POST body / json) | ||
* ``IsCsrfTokenValid::SOURCE_QUERY``: query string | ||
* ``IsCsrfTokenValid::SOURCE_HEADER``: request headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* ``IsCsrfTokenValid::SOURCE_HEADER``: request headers | |
* ``IsCsrfTokenValid::SOURCE_HEADER``: request header |
As it checks only one no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Fixed while merging. Thanks!
#[IsCsrfTokenValid( | ||
'delete-item', | ||
tokenKey: 'token', | ||
tokenSource: IsCsrfTokenValid::SOURCE_PAYLOAD | IsCsrfTokenValid::SOURCE_QUERY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion, but perhaps overkill
Document one simple source and one with combination?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it "as is" because I think it's simple enough to understand it. Thanks.
Thanks David! |
Fix #21362